-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Search Session] Fix integration in vega, timelion and TSVB #87862
Conversation
Pinging @elastic/kibana-app-services (Team:AppServices) |
@@ -93,7 +96,15 @@ export function getTimelionRequestHandler({ | |||
|
|||
// parse the time range client side to make sure it behaves like other charts | |||
const timeRangeBounds = timefilter.calculateBounds(timeRange); | |||
const sessionId = getDataSearch().session.getSessionId(); | |||
const isCurrentSession = () => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just interesting, why you defined isCurrentSession
as a function and execute it each time.
can we do something like: const isCurrentSession = !!searchSessionId && searchSessionId === dataSearch.session.getSessionId();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to "recalculate" it in finally
clause because it could already be that when async call finishes dataSearch.session.getSessionId();
has changed.
technically it won't be a problem to call untrack
in that case either, but I am doing it for sanity, mostly to make it easier to debug the internal current session state by not calling untrack
where we know it is not necessary
src/plugins/vis_type_timelion/public/helpers/timelion_request_handler.ts
Outdated
Show resolved
Hide resolved
fetchMock.mockReset(); | ||
}); | ||
|
||
test('infers isRestore from session service state', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests were moved into session_service
together with underlying logic.
src/plugins/vis_type_timelion/public/helpers/timelion_request_handler.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM in case of green CI
…sc-integrations # Conflicts: # src/plugins/data/public/search/session/session_service.test.ts # src/plugins/data/public/search/session/session_service.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kibana app code changes LGTM, thanx for that @Dosant!
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't checked out.
Overall LGTM
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Page load bundle
History
To update your PR or re-run it, just comment with: |
Summary
This pr address some search session integration issues in vega, timelion, and TSVB panels.
Part of #61738
Follow up on #82835, #82232, #82229
sessionId
received from expression pipeline instead of getting it from global data serviceisRestore
andisStored
boolean flags to the servertrackSearch/untrackSearch
on a sessionHow to test
In
kibana.yml
:dashboardUrl&searchSessionId={your-id}
to make sure we are actually restoring data for all the panels try augmenting the dashboard state in the URL, e.g. changing time, or use a fake session id in the URL. All panels should fail.
For maintainers